ymandel updated this revision to Diff 197250.
ymandel added a comment.
Herald added a subscriber: jfb.
Add test for new behavior.
In the process, tweak the handling of errors from TextGenerators in Transformer:
instead of printing to `llvm::errs`, we set the error in the AtomicChange.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D61015/new/
https://reviews.llvm.org/D61015
Files:
clang/include/clang/Tooling/Refactoring/Transformer.h
clang/lib/Tooling/Refactoring/Transformer.cpp
clang/unittests/Tooling/TransformerTest.cpp
Index: clang/unittests/Tooling/TransformerTest.cpp
===================================================================
--- clang/unittests/Tooling/TransformerTest.cpp
+++ clang/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"
@@ -356,6 +358,27 @@
// Negative tests (where we expect no transformation to occur).
//
+// 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::make_error<llvm::StringError>(llvm::errc::invalid_argument,
+ "ERROR");
+ };
+ Transformer T(makeRule(binaryOperator().bind(O), change<Expr>(O, AlwaysFail)),
+ [this](const AtomicChange &C) { Changes.push_back(C); });
+ T.registerMatchers(&MatchFinder);
+ // Rewriting doesn't change the input, but produces an AtomicChange that
+ // carries an error (`rewrite()` doesn't fail outright, because it doesn't
+ // check for errors in the changes).
+ compareSnippets(Input, rewrite(Input));
+ ASSERT_EQ(Changes.size(), 1u);
+ EXPECT_TRUE(Changes[0].hasError());
+}
+
// 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; }";
Index: clang/lib/Tooling/Refactoring/Transformer.cpp
===================================================================
--- clang/lib/Tooling/Refactoring/Transformer.cpp
+++ clang/lib/Tooling/Refactoring/Transformer.cpp
@@ -157,12 +157,16 @@
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()))
+ auto &Range = *RangeOrErr;
+ if (Range.isInvalid() ||
+ isOriginMacroBody(*Result.SourceManager, Range.getBegin()))
return SmallVector<Transformation, 0>();
- T.Replacement = Edit.Replacement(Result);
+ auto ReplacementOrErr = Edit.Replacement(Result);
+ if (auto Err = ReplacementOrErr.takeError())
+ return std::move(Err);
+ Transformation T;
+ T.Range = Range;
+ T.Replacement = std::move(*ReplacementOrErr);
Transformations.push_back(std::move(T));
}
return Transformations;
@@ -194,12 +198,15 @@
Root->second.getSourceRange().getBegin());
assert(RootLoc.isValid() && "Invalid location for Root node of match.");
+ AtomicChange AC(*Result.SourceManager, RootLoc);
+
auto TransformationsOrErr = translateEdits(Result, Rule.Edits);
if (auto Err = TransformationsOrErr.takeError()) {
- llvm::errs() << "Transformation failed: " << llvm::toString(std::move(Err))
- << "\n";
+ AC.setError(llvm::toString(std::move(Err)));
+ Consumer(AC);
return;
}
+
auto &Transformations = *TransformationsOrErr;
if (Transformations.empty()) {
// No rewrite applied (but no error encountered either).
@@ -209,8 +216,7 @@
return;
}
- // Convert the result to an AtomicChange.
- AtomicChange AC(*Result.SourceManager, RootLoc);
+ // Record the results in the AtomicChange.
for (const auto &T : Transformations) {
if (auto Err = AC.replace(*Result.SourceManager, T.Range, T.Replacement)) {
AC.setError(llvm::toString(std::move(Err)));
Index: clang/include/clang/Tooling/Refactoring/Transformer.h
===================================================================
--- clang/include/clang/Tooling/Refactoring/Transformer.h
+++ clang/include/clang/Tooling/Refactoring/Transformer.h
@@ -44,12 +44,21 @@
Name,
};
-using TextGenerator =
- std::function<std::string(const ast_matchers::MatchFinder::MatchResult &)>;
+// \c TextGenerator may fail, because it processes dynamically-bound match
+// results. For example, a typo in the name of a bound node or a mismatch in
+// the node's type can lead to a failure in the string generation code. We
+// allow the generator to return \c Expected, rather than assert on such
+// failures, so that the Transformer client can choose how to handle the error.
+// For example, if used in a UI (for example, clang-query or a web app), in
+// which the user specifies the rewrite rule, the backend might choose to return
+// a diagnostic error, rather than crash.
+using TextGenerator = std::function<Expected<std::string>(
+ const ast_matchers::MatchFinder::MatchResult &)>;
-/// Wraps a string as a TextGenerator.
+/// Wraps a string as a (trivially successful) 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.
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
