[clang][transformer] Allow usage of applyFirst with rewriteDescendants #117658
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-clang Author: None (SherAndrei) ChangesWithout these changes a clash appears between a Tag, which is bound to enclosing match, and a Tag, which is associated with first Case of applyFirst in rewriteDescendands. We fix this by making sure that associated Tags are unique and deterministic as they are intend to be. Full diff: https://github.com/llvm/llvm-project/pull/117658.diff 2 Files Affected:
diff --git a/clang/lib/Tooling/Transformer/RewriteRule.cpp b/clang/lib/Tooling/Transformer/RewriteRule.cpp
index eefddc34940487..196249260ec8b4 100644
--- a/clang/lib/Tooling/Transformer/RewriteRule.cpp
+++ b/clang/lib/Tooling/Transformer/RewriteRule.cpp
@@ -382,9 +382,10 @@ static std::vector<DynTypedMatcher> taggedMatchers(
std::vector<DynTypedMatcher> Matchers;
Matchers.reserve(Cases.size());
for (const auto &Case : Cases) {
- std::string Tag = (TagBase + Twine(Case.first)).str();
// HACK: Many matchers are not bindable, so ensure that tryBind will work.
DynTypedMatcher BoundMatcher(Case.second.Matcher);
+ const auto [_, ID] = BoundMatcher.getID();
+ std::string Tag = (TagBase + Twine(ID)).str();
BoundMatcher.setAllowBind(true);
auto M = *BoundMatcher.tryBind(Tag);
Matchers.push_back(!M.getTraversalKind()
@@ -469,7 +470,8 @@ size_t transformer::detail::findSelectedCase(const MatchResult &Result,
auto &NodesMap = Result.Nodes.getMap();
for (size_t i = 0, N = Rule.Cases.size(); i < N; ++i) {
- std::string Tag = ("Tag" + Twine(i)).str();
+ const auto [_, ID] = Rule.Cases[i].Matcher.getID();
+ std::string Tag = ("Tag" + Twine(ID)).str();
if (NodesMap.find(Tag) != NodesMap.end())
return i;
}
diff --git a/clang/unittests/Tooling/TransformerTest.cpp b/clang/unittests/Tooling/TransformerTest.cpp
index cbd84ab794a49a..0404c81862468f 100644
--- a/clang/unittests/Tooling/TransformerTest.cpp
+++ b/clang/unittests/Tooling/TransformerTest.cpp
@@ -605,6 +605,49 @@ TEST_F(TransformerTest, RewriteDescendantsReferToParentBinding) {
Input, Expected);
}
+TEST_F(TransformerTest, RewriteDescendantsApplyFirstOrderedRuleUnrelated) {
+ std::string Input = "int f(int x) { int y = x; return x; }";
+ std::string Expected = "int f(int x) { char y = 3; return 3; }";
+ auto IntToChar = makeRule(typeLoc(loc(qualType(isInteger(), builtinType()))),
+ changeTo(cat("char")));
+ auto InlineX =
+ makeRule(declRefExpr(to(varDecl(hasName("x")))), changeTo(cat("3")));
+ testRule(
+ makeRule(functionDecl(hasName("f"), hasBody(stmt().bind("body"))),
+ rewriteDescendants("body", applyFirst({InlineX, IntToChar}))),
+ Input, Expected);
+}
+
+TEST_F(TransformerTest, RewriteDescendantsApplyFirstOrderedRuleRelated) {
+ std::string Input = "int f(int x) { int y = x; return x; }";
+ std::string Expected = "int f(int x) { int y = 3; return y; }";
+ auto ReturnY = makeRule(
+ traverse(TK_IgnoreUnlessSpelledInSource,
+ declRefExpr(to(varDecl(hasName("x"))), hasParent(returnStmt()))),
+ changeTo(cat("y")));
+ auto InlineX = makeRule(traverse(TK_IgnoreUnlessSpelledInSource,
+ declRefExpr(to(varDecl(hasName("x"))))),
+ changeTo(cat("3")));
+ testRule(makeRule(functionDecl(hasName("f"), hasBody(stmt().bind("body"))),
+ rewriteDescendants("body", applyFirst({ReturnY, InlineX}))),
+ Input, Expected);
+}
+
+TEST_F(TransformerTest, RewriteDescendantsApplyFirstOrderedRuleRelatedSwapped) {
+ std::string Input = "int f(int x) { int y = x; return x; }";
+ std::string Expected = "int f(int x) { int y = 3; return 3; }";
+ auto ReturnY = makeRule(
+ traverse(TK_IgnoreUnlessSpelledInSource,
+ declRefExpr(to(varDecl(hasName("x"))), hasParent(returnStmt()))),
+ changeTo(cat("y")));
+ auto InlineX = makeRule(traverse(TK_IgnoreUnlessSpelledInSource,
+ declRefExpr(to(varDecl(hasName("x"))))),
+ changeTo(cat("3")));
+ testRule(makeRule(functionDecl(hasName("f"), hasBody(stmt().bind("body"))),
+ rewriteDescendants("body", applyFirst({InlineX, ReturnY}))),
+ Input, Expected);
+}
+
TEST_F(TransformerTest, RewriteDescendantsUnboundNode) {
std::string Input =
"int f(int x) { int y = x; { int z = x * x; } return x; }";
|
|
@ymand, can you please review? |
Without these changes a clash appears between a Tag, which is bound to enclosing match, and a Tag, which is associated with first Case of applyFirst in rewriteDescendands. We fix this by making sure that associated Tags are unique and deterministic as they are intend to be.
SherAndrei
force-pushed
the
rewrite-descendantds-apply-first
branch
from
9634a78 to
5eea966
Compare
SherAndrei
changed the title
|
@ymand, could you please take a look? |
|
@5chmidti, @carlosgalvezp, @PiotrZSL, can anyone please review suggested changes or maybe help find who can additionally review them? The author of the code under question -- @ymand, is absent for over a week |
Yes -- sorry, missed the notification. I'll try to review within 24h. |
Can you expand on your concern about the clash? Your approach of using the matcher ID seems like an improvement, but I'm not clear on what exactly we're trying to avoid. |
The clash is happening in
Have I made anything clearer? |
Thanks for the explanation (and for your patience). Yes, this would be good to specify in the description. That said, I'm still a little unclear:
|
Yes, two of them do fail if no suggested changes are present. Feel free to reproduce it yourself. ./tools/clang/unittests/Tooling/ToolingTests --gtest_filter=*Transform*RewriteDescendantsApplyFirst*yields Note: Google Test filter = *Transform*RewriteDescendantsApplyFirst*
[==========] Running 3 tests from 1 test suite.
[----------] Global test environment set-up.
[----------] 3 tests from TransformerTest
[ RUN ] TransformerTest.RewriteDescendantsApplyFirstOrderedRuleUnrelated
/root/clang-llvm/llvm-project/clang/unittests/Tooling/TransformerTest.cpp:94: Failure
Expected equality of these values:
format(Expected)
Which is: "int f(int x) {\n char y = 3;\n return 3;\n}"
format(Actual)
Which is: "int f(int x) {\n 3 y = 3;\n return 3;\n}"
With diff:
@@ -1,4 +1,4 @@
int f(int x) {
- char y = 3;
+ 3 y = 3;
return 3;
}
[ FAILED ] TransformerTest.RewriteDescendantsApplyFirstOrderedRuleUnrelated (17 ms)
[ RUN ] TransformerTest.RewriteDescendantsApplyFirstOrderedRuleRelated
/root/clang-llvm/llvm-project/clang/unittests/Tooling/TransformerTest.cpp:94: Failure
Expected equality of these values:
format(Expected)
Which is: "int f(int x) {\n int y = 3;\n return y;\n}"
format(Actual)
Which is: "int f(int x) {\n int y = y;\n return y;\n}"
With diff:
@@ -1,4 +1,4 @@
int f(int x) {
- int y = 3;
+ int y = y;
return y;
}
[ FAILED ] TransformerTest.RewriteDescendantsApplyFirstOrderedRuleRelated (14 ms)
[ RUN ] TransformerTest.RewriteDescendantsApplyFirstOrderedRuleRelatedSwapped
[ OK ] TransformerTest.RewriteDescendantsApplyFirstOrderedRuleRelatedSwapped (13 ms)
[----------] 3 tests from TransformerTest (45 ms total)
[----------] Global test environment tear-down
[==========] 3 tests from 1 test suite ran. (45 ms total)
[ PASSED ] 1 test.
[ FAILED ] 2 tests, listed below:
[ FAILED ] TransformerTest.RewriteDescendantsApplyFirstOrderedRuleUnrelated
[ FAILED ] TransformerTest.RewriteDescendantsApplyFirstOrderedRuleRelated
2 FAILED TESTS
Yes, indeed the original match happens in separate context, but all contexts share one
Now let's consider same test with suggested changes
Sorry for misleading you by using the word 'shifted' here. Nothing is shifted here really. I do struggle to update description of the commit. What do you think about next description of the commit: |
I see -- I'd missed the reuse of the MatchResult. Now that I understand the problem, I don't think that you're suggestion will fix it. The problem is that the IDs of the matchers are unique between different matchers, but not necessarily unique within a rule, because you can trivially reuse a matcher twice within the rule. So, we need a different source of uniqueness. I don't have a quick fix offhand -- I think we'd probably want to package the ID with the rule case and then use a unique-ID generator (e.g. a global int variable) when we create the cases. But, in the meantime, if you want to unblock your progress, you could combine the position-based ID with the matcher-based ID, which would significantly decrease the likelihood of conflict. But, if you do that, please include a simple test that demonstrates the problem. thanks! |
3 participants
Without these changes a clash appears between a Tag, which is bound to enclosing match, and a Tag, which is associated with first Case of applyFirst in rewriteDescendands.
We fix this by making sure that associated Tags are unique and deterministic as they are intend to be.