r359574 - [LibTooling] Change Transformer's TextGenerator to a partial function.
Yitzhak Mandelbaum via cfe-commits
cfe-commits at lists.llvm.org
Tue Apr 30 09:48:33 PDT 2019
Author: ymandel
Date: Tue Apr 30 09:48:33 2019
New Revision: 359574
URL: http://llvm.org/viewvc/llvm-project?rev=359574&view=rev
Log:
[LibTooling] Change Transformer's TextGenerator to a partial function.
Summary:
Changes the signature of the TextGenerator std::function to return an Expected<std::string>
instead of std::string to allow for (non-fatal) failures. Previously, we
expected that any failures would be expressed with assertions. However, that's
unfriendly to running the code in servers or other places that don't want their
library calls to crash the program.
Correspondingly, updates Transformer's handling of failures in TextGenerators
and the signature of `ChangeConsumer`.
Reviewers: ilya-biryukov
Subscribers: cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D61015
Modified:
cfe/trunk/include/clang/Tooling/Refactoring/Transformer.h
cfe/trunk/lib/Tooling/Refactoring/Transformer.cpp
cfe/trunk/unittests/Tooling/TransformerTest.cpp
Modified: cfe/trunk/include/clang/Tooling/Refactoring/Transformer.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Tooling/Refactoring/Transformer.h?rev=359574&r1=359573&r2=359574&view=diff
==============================================================================
--- cfe/trunk/include/clang/Tooling/Refactoring/Transformer.h (original)
+++ cfe/trunk/include/clang/Tooling/Refactoring/Transformer.h Tue Apr 30 09:48:33 2019
@@ -44,12 +44,16 @@ enum class NodePart {
Name,
};
-using TextGenerator =
- std::function<std::string(const ast_matchers::MatchFinder::MatchResult &)>;
+// Note that \p TextGenerator is allowed to fail, e.g. when trying to access a
+// matched node that was not bound. Allowing this to fail simplifies error
+// handling for interactive tools like clang-query.
+using TextGenerator = std::function<Expected<std::string>(
+ const ast_matchers::MatchFinder::MatchResult &)>;
/// Wraps a string as a TextGenerator.
inline TextGenerator text(std::string M) {
- return [M](const ast_matchers::MatchFinder::MatchResult &) { return M; };
+ return [M](const ast_matchers::MatchFinder::MatchResult &)
+ -> Expected<std::string> { return M; };
}
// Description of a source-code edit, expressed in terms of an AST node.
@@ -222,11 +226,13 @@ translateEdits(const ast_matchers::Match
class Transformer : public ast_matchers::MatchFinder::MatchCallback {
public:
using ChangeConsumer =
- std::function<void(const clang::tooling::AtomicChange &Change)>;
+ std::function<void(Expected<clang::tooling::AtomicChange> Change)>;
- /// \param Consumer Receives each successful rewrite as an \c AtomicChange.
- /// Note that clients are responsible for handling the case that independent
- /// \c AtomicChanges conflict with each other.
+ /// \param Consumer Receives each rewrite or error. Will not necessarily be
+ /// called for each match; for example, if the rewrite is not applicable
+ /// because of macros, but doesn't fail. Note that clients are responsible
+ /// for handling the case that independent \c AtomicChanges conflict with each
+ /// other.
Transformer(RewriteRule Rule, ChangeConsumer Consumer)
: Rule(std::move(Rule)), Consumer(std::move(Consumer)) {}
Modified: cfe/trunk/lib/Tooling/Refactoring/Transformer.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Tooling/Refactoring/Transformer.cpp?rev=359574&r1=359573&r2=359574&view=diff
==============================================================================
--- cfe/trunk/lib/Tooling/Refactoring/Transformer.cpp (original)
+++ cfe/trunk/lib/Tooling/Refactoring/Transformer.cpp Tue Apr 30 09:48:33 2019
@@ -153,16 +153,19 @@ tooling::translateEdits(const MatchResul
auto It = NodesMap.find(Edit.Target);
assert(It != NodesMap.end() && "Edit target must be bound in the match.");
- Expected<CharSourceRange> RangeOrErr = getTargetRange(
+ Expected<CharSourceRange> Range = getTargetRange(
Edit.Target, It->second, Edit.Kind, Edit.Part, *Result.Context);
- if (auto Err = RangeOrErr.takeError())
- return std::move(Err);
- Transformation T;
- T.Range = *RangeOrErr;
- if (T.Range.isInvalid() ||
- isOriginMacroBody(*Result.SourceManager, T.Range.getBegin()))
+ if (!Range)
+ return Range.takeError();
+ if (Range->isInvalid() ||
+ isOriginMacroBody(*Result.SourceManager, Range->getBegin()))
return SmallVector<Transformation, 0>();
- T.Replacement = Edit.Replacement(Result);
+ auto Replacement = Edit.Replacement(Result);
+ if (!Replacement)
+ return Replacement.takeError();
+ Transformation T;
+ T.Range = *Range;
+ T.Replacement = std::move(*Replacement);
Transformations.push_back(std::move(T));
}
return Transformations;
@@ -194,14 +197,13 @@ void Transformer::run(const MatchResult
Root->second.getSourceRange().getBegin());
assert(RootLoc.isValid() && "Invalid location for Root node of match.");
- auto TransformationsOrErr = translateEdits(Result, Rule.Edits);
- if (auto Err = TransformationsOrErr.takeError()) {
- llvm::errs() << "Transformation failed: " << llvm::toString(std::move(Err))
- << "\n";
+ auto Transformations = translateEdits(Result, Rule.Edits);
+ if (!Transformations) {
+ Consumer(Transformations.takeError());
return;
}
- auto &Transformations = *TransformationsOrErr;
- if (Transformations.empty()) {
+
+ if (Transformations->empty()) {
// No rewrite applied (but no error encountered either).
RootLoc.print(llvm::errs() << "note: skipping match at loc ",
*Result.SourceManager);
@@ -209,14 +211,14 @@ void Transformer::run(const MatchResult
return;
}
- // Convert the result to an AtomicChange.
+ // Record the results in the AtomicChange.
AtomicChange AC(*Result.SourceManager, RootLoc);
- for (const auto &T : Transformations) {
+ for (const auto &T : *Transformations) {
if (auto Err = AC.replace(*Result.SourceManager, T.Range, T.Replacement)) {
- AC.setError(llvm::toString(std::move(Err)));
- break;
+ Consumer(std::move(Err));
+ return;
}
}
- Consumer(AC);
+ Consumer(std::move(AC));
}
Modified: cfe/trunk/unittests/Tooling/TransformerTest.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Tooling/TransformerTest.cpp?rev=359574&r1=359573&r2=359574&view=diff
==============================================================================
--- cfe/trunk/unittests/Tooling/TransformerTest.cpp (original)
+++ cfe/trunk/unittests/Tooling/TransformerTest.cpp Tue Apr 30 09:48:33 2019
@@ -10,6 +10,8 @@
#include "clang/ASTMatchers/ASTMatchers.h"
#include "clang/Tooling/Tooling.h"
+#include "llvm/Support/Errc.h"
+#include "llvm/Support/Error.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
@@ -18,6 +20,8 @@ using namespace tooling;
using namespace ast_matchers;
namespace {
+using ::testing::IsEmpty;
+
constexpr char KHeaderContents[] = R"cc(
struct string {
string(const char*);
@@ -84,26 +88,43 @@ protected:
Factory->create(), Code, std::vector<std::string>(), "input.cc",
"clang-tool", std::make_shared<PCHContainerOperations>(),
FileContents)) {
+ llvm::errs() << "Running tool failed.\n";
+ return None;
+ }
+ if (ErrorCount != 0) {
+ llvm::errs() << "Generating changes failed.\n";
return None;
}
- auto ChangedCodeOrErr =
+ auto ChangedCode =
applyAtomicChanges("input.cc", Code, Changes, ApplyChangesSpec());
- if (auto Err = ChangedCodeOrErr.takeError()) {
- llvm::errs() << "Change failed: " << llvm::toString(std::move(Err))
- << "\n";
+ if (!ChangedCode) {
+ llvm::errs() << "Applying changes failed: "
+ << llvm::toString(ChangedCode.takeError()) << "\n";
return None;
}
- return *ChangedCodeOrErr;
+ return *ChangedCode;
+ }
+
+ Transformer::ChangeConsumer consumer() {
+ return [this](Expected<AtomicChange> C) {
+ if (C) {
+ Changes.push_back(std::move(*C));
+ } else {
+ consumeError(C.takeError());
+ ++ErrorCount;
+ }
+ };
}
void testRule(RewriteRule Rule, StringRef Input, StringRef Expected) {
- Transformer T(std::move(Rule),
- [this](const AtomicChange &C) { Changes.push_back(C); });
+ Transformer T(std::move(Rule), consumer());
T.registerMatchers(&MatchFinder);
compareSnippets(Expected, rewrite(Input));
}
clang::ast_matchers::MatchFinder MatchFinder;
+ // Records whether any errors occurred in individual changes.
+ int ErrorCount = 0;
AtomicChanges Changes;
private:
@@ -357,6 +378,23 @@ TEST_F(TransformerTest, MultiChange) {
//
// Tests for a conflict in edits from a single match for a rule.
+TEST_F(TransformerTest, TextGeneratorFailure) {
+ std::string Input = "int conflictOneRule() { return 3 + 7; }";
+ // Try to change the whole binary-operator expression AND one its operands:
+ StringRef O = "O";
+ auto AlwaysFail = [](const ast_matchers::MatchFinder::MatchResult &)
+ -> llvm::Expected<std::string> {
+ return llvm::createStringError(llvm::errc::invalid_argument, "ERROR");
+ };
+ Transformer T(makeRule(binaryOperator().bind(O), change<Expr>(O, AlwaysFail)),
+ consumer());
+ T.registerMatchers(&MatchFinder);
+ EXPECT_FALSE(rewrite(Input));
+ EXPECT_THAT(Changes, IsEmpty());
+ EXPECT_EQ(ErrorCount, 1);
+}
+
+// Tests for a conflict in edits from a single match for a rule.
TEST_F(TransformerTest, OverlappingEditsInRule) {
std::string Input = "int conflictOneRule() { return 3 + 7; }";
// Try to change the whole binary-operator expression AND one its operands:
@@ -364,13 +402,11 @@ TEST_F(TransformerTest, OverlappingEdits
Transformer T(
makeRule(binaryOperator(hasLHS(expr().bind(L))).bind(O),
{change<Expr>(O, "DELETE_OP"), change<Expr>(L, "DELETE_LHS")}),
- [this](const AtomicChange &C) { Changes.push_back(C); });
+ consumer());
T.registerMatchers(&MatchFinder);
- // The rewrite process fails...
- EXPECT_TRUE(rewrite(Input));
- // ... but one AtomicChange was consumed:
- ASSERT_EQ(Changes.size(), 1u);
- EXPECT_TRUE(Changes[0].hasError());
+ EXPECT_FALSE(rewrite(Input));
+ EXPECT_THAT(Changes, IsEmpty());
+ EXPECT_EQ(ErrorCount, 1);
}
// Tests for a conflict in edits across multiple matches (of the same rule).
@@ -379,27 +415,27 @@ TEST_F(TransformerTest, OverlappingEdits
// Try to change the whole binary-operator expression AND one its operands:
StringRef E = "E";
Transformer T(makeRule(expr().bind(E), change<Expr>(E, "DELETE_EXPR")),
- [this](const AtomicChange &C) { Changes.push_back(C); });
+ consumer());
T.registerMatchers(&MatchFinder);
// The rewrite process fails because the changes conflict with each other...
EXPECT_FALSE(rewrite(Input));
- // ... but all changes are (individually) fine:
- ASSERT_EQ(Changes.size(), 2u);
- EXPECT_FALSE(Changes[0].hasError());
- EXPECT_FALSE(Changes[1].hasError());
+ // ... but two changes were produced.
+ EXPECT_EQ(Changes.size(), 2u);
+ EXPECT_EQ(ErrorCount, 0);
}
TEST_F(TransformerTest, ErrorOccurredMatchSkipped) {
// Syntax error in the function body:
std::string Input = "void errorOccurred() { 3 }";
- Transformer T(
- makeRule(functionDecl(hasName("errorOccurred")), change<Decl>("DELETED;")),
- [this](const AtomicChange &C) { Changes.push_back(C); });
+ Transformer T(makeRule(functionDecl(hasName("errorOccurred")),
+ change<Decl>("DELETED;")),
+ consumer());
T.registerMatchers(&MatchFinder);
// The rewrite process itself fails...
EXPECT_FALSE(rewrite(Input));
- // ... and no changes are produced in the process.
- EXPECT_THAT(Changes, ::testing::IsEmpty());
+ // ... and no changes or errors are produced in the process.
+ EXPECT_THAT(Changes, IsEmpty());
+ EXPECT_EQ(ErrorCount, 0);
}
TEST_F(TransformerTest, NoTransformationInMacro) {
More information about the cfe-commits
mailing list