[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