[clang] [clang][transformer] Allow usage of applyFirst with rewriteDescendants (PR #117658)
via cfe-commits
cfe-commits at lists.llvm.org
Wed Dec 25 14:54:56 PST 2024
SherAndrei wrote:
> Will any of the new tests trigger the bug?
Yes, two of them do fail if no suggested changes are present. Feel free to reproduce it yourself.
```bash
./tools/clang/unittests/Tooling/ToolingTests --gtest_filter=*Transform*RewriteDescendantsApplyFirst*
```
yields
```diff
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
```
> Can you clarify where the matchers associated with the nested rule will be shifted? As far as I can tell, the original match uses Tag0 but happens in a separate context (that is, MatchFinder) than the nested match so I don't see how they are interacting.
Yes, indeed the original match happens in separate context, but all contexts share one `NodesMap`,
which values are overriden with a consecutive match ('clash'). Let's consider test
`RewriteDescendantsApplyFirstOrderedRuleUnrelated` before suggested changes:
1. `Transformer::registerMatchers` calls to `taggedMatchers`: `FunctionDecl` matcher, we'll call it FD, receives "Tag0",
2. then `ApplyRuleCallback::registerMatchers` calls to `taggedMatchers`: first `DeclRefExpr` matcher, let's call it DRE1, receives "Tag0", second `DeclRefExpr` matcher -- DRE2 -- receives "Tag1",
3. when DRE1 matches, `ApplyRuleCallback::run` calls to `findSelectedCase`: it successfully finds "Tag0" which is bound to FD and not to expected DRE1.
Now let's consider same test with suggested changes
1. `Transformer::registerMatchers` binds FD to "Tag93825274205568",
2. `ApplyRuleCallback::registerMatchers` binds DRE1 to "Tag93825274195136", DRE2 to "Tag93825274196288",
3. when DRE1 matches, `findSelectedCase` finds expected DRE1 by searching for "Tag93825274195136"
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:
```
[clang][transformer] Allow usage of applyFirst with rewriteDescendants
Previously,, `taggedMatchers` could apply same tag to two different matchers (here, for enclosing matcher and for first from `applyFirst`).
We fix this by making sure that associated Tags are unique and deterministic as they are intend to be.
```
https://github.com/llvm/llvm-project/pull/117658
More information about the cfe-commits
mailing list