Index: cfe/trunk/include/clang/Tooling/Refactoring/Transformer.h =================================================================== --- cfe/trunk/include/clang/Tooling/Refactoring/Transformer.h +++ cfe/trunk/include/clang/Tooling/Refactoring/Transformer.h @@ -44,12 +44,16 @@ Name, }; -using TextGenerator = - std::function; +// 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( + 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 { return M; }; } // Description of a source-code edit, expressed in terms of an AST node. @@ -222,11 +226,13 @@ class Transformer : public ast_matchers::MatchFinder::MatchCallback { public: using ChangeConsumer = - std::function; + std::function 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)) {} Index: cfe/trunk/lib/Tooling/Refactoring/Transformer.cpp =================================================================== --- cfe/trunk/lib/Tooling/Refactoring/Transformer.cpp +++ cfe/trunk/lib/Tooling/Refactoring/Transformer.cpp @@ -153,16 +153,19 @@ auto It = NodesMap.find(Edit.Target); assert(It != NodesMap.end() && "Edit target must be bound in the match."); - Expected RangeOrErr = getTargetRange( + Expected 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(); - 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 @@ 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 @@ 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)); } Index: cfe/trunk/unittests/Tooling/TransformerTest.cpp =================================================================== --- cfe/trunk/unittests/Tooling/TransformerTest.cpp +++ cfe/trunk/unittests/Tooling/TransformerTest.cpp @@ -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 ast_matchers; namespace { +using ::testing::IsEmpty; + constexpr char KHeaderContents[] = R"cc( struct string { string(const char*); @@ -84,26 +88,43 @@ Factory->create(), Code, std::vector(), "input.cc", "clang-tool", std::make_shared(), 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 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 @@ // // 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 { + return llvm::createStringError(llvm::errc::invalid_argument, "ERROR"); + }; + Transformer T(makeRule(binaryOperator().bind(O), change(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 @@ Transformer T( makeRule(binaryOperator(hasLHS(expr().bind(L))).bind(O), {change(O, "DELETE_OP"), change(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 @@ // Try to change the whole binary-operator expression AND one its operands: StringRef E = "E"; Transformer T(makeRule(expr().bind(E), change(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("DELETED;")), - [this](const AtomicChange &C) { Changes.push_back(C); }); + Transformer T(makeRule(functionDecl(hasName("errorOccurred")), + change("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) {