[clang-tools-extra] d8c1f43 - [libTooling] Move RewriteRule include edits to ASTEdit granularity.
Yitzhak Mandelbaum via cfe-commits
cfe-commits at lists.llvm.org
Tue Aug 11 09:48:06 PDT 2020
Author: Yitzhak Mandelbaum
Date: 2020-08-11T16:47:14Z
New Revision: d8c1f43dcc949fda5ce37a122d1a0d92975de82c
URL: https://github.com/llvm/llvm-project/commit/d8c1f43dcc949fda5ce37a122d1a0d92975de82c
DIFF: https://github.com/llvm/llvm-project/commit/d8c1f43dcc949fda5ce37a122d1a0d92975de82c.diff
LOG: [libTooling] Move RewriteRule include edits to ASTEdit granularity.
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
Added:
Modified:
clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
clang/include/clang/Tooling/Transformer/RewriteRule.h
clang/lib/Tooling/Transformer/RewriteRule.cpp
clang/lib/Tooling/Transformer/Transformer.cpp
clang/unittests/Tooling/TransformerTest.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp b/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
index 2c116b210d05..6ea757a38f05 100644
--- a/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
+++ b/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
@@ -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(
@@ -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(
diff --git a/clang/include/clang/Tooling/Transformer/RewriteRule.h b/clang/include/clang/Tooling/Transformer/RewriteRule.h
index ac4d19fd28ca..9700d1ff539d 100644
--- a/clang/include/clang/Tooling/Transformer/RewriteRule.h
+++ b/clang/include/clang/Tooling/Transformer/RewriteRule.h
@@ -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
@@ -86,6 +102,7 @@ using AnyGenerator = MatchConsumer<llvm::Any>;
// changeTo(cat("
diff erent_expr"))
// \endcode
struct ASTEdit {
+ EditKind Kind = EditKind::Range;
RangeSelector TargetRange;
TextGenerator Replacement;
TextGenerator Note;
@@ -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.
@@ -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
@@ -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
diff erent 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;
diff --git a/clang/lib/Tooling/Transformer/RewriteRule.cpp b/clang/lib/Tooling/Transformer/RewriteRule.cpp
index 23c63904539d..8781bfc66146 100644
--- a/clang/lib/Tooling/Transformer/RewriteRule.cpp
+++ b/clang/lib/Tooling/Transformer/RewriteRule.cpp
@@ -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);
@@ -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 {
@@ -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
diff --git a/clang/lib/Tooling/Transformer/Transformer.cpp b/clang/lib/Tooling/Transformer/Transformer.cpp
index 5b5be7a396db..7a4d8b45f189 100644
--- a/clang/lib/Tooling/Transformer/Transformer.cpp
+++ b/clang/lib/Tooling/Transformer/Transformer.cpp
@@ -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));
}
diff --git a/clang/unittests/Tooling/TransformerTest.cpp b/clang/unittests/Tooling/TransformerTest.cpp
index 77fd380410b2..26158b1520f9 100644
--- a/clang/unittests/Tooling/TransformerTest.cpp
+++ b/clang/unittests/Tooling/TransformerTest.cpp
@@ -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;
@@ -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");
@@ -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);
@@ -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
More information about the cfe-commits
mailing list