Skip to content

Commit

Permalink
[clang][ASTMatcher] Fix execution order of hasOperands submatchers (l…
Browse files Browse the repository at this point in the history
…lvm#104148)

`hasOperands` does not always execute matchers in the order they are
written. This can cause issue in code using bindings when one operand
matcher is relying on a binding set by the other. With this change, the
first matcher present in the code is always executed first and any
binding it sets are available to the second matcher.

Simple example with current version (1 match) and new version (2
matches):
```bash
> cat tmp.cpp
int a = 13;
int b = ((int) a) - a;
int c = a - ((int) a);

> clang-query tmp.cpp
clang-query> set traversal IgnoreUnlessSpelledInSource
clang-query> m binaryOperator(hasOperands(cStyleCastExpr(has(declRefExpr(hasDeclaration(valueDecl().bind("d"))))), declRefExpr(hasDeclaration(valueDecl(equalsBoundNode("d"))))))

Match #1:

tmp.cpp:1:1: note: "d" binds here
int a = 13;
^~~~~~~~~~
tmp.cpp:2:9: note: "root" binds here
int b = ((int)a) - a;
        ^~~~~~~~~~~~
1 match.

> ./build/bin/clang-query tmp.cpp
clang-query> set traversal IgnoreUnlessSpelledInSource
clang-query> m binaryOperator(hasOperands(cStyleCastExpr(has(declRefExpr(hasDeclaration(valueDecl().bind("d"))))), declRefExpr(hasDeclaration(valueDecl(equalsBoundNode("d"))))))

Match #1:

tmp.cpp:1:1: note: "d" binds here
    1 | int a = 13;
      | ^~~~~~~~~~
tmp.cpp:2:9: note: "root" binds here
    2 | int b = ((int)a) - a;
      |         ^~~~~~~~~~~~

Match #2:

tmp.cpp:1:1: note: "d" binds here
    1 | int a = 13;
      | ^~~~~~~~~~
tmp.cpp:3:9: note: "root" binds here
    3 | int c = a - ((int)a);
      |         ^~~~~~~~~~~~
2 matches.
```

If this should be documented or regression tested anywhere please let me
know where.
  • Loading branch information
nicovank authored Aug 20, 2024
1 parent 90a8e5a commit f9e2a86
Show file tree
Hide file tree
Showing 3 changed files with 16 additions and 1 deletion.
3 changes: 3 additions & 0 deletions clang/docs/ReleaseNotes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -388,6 +388,9 @@ AST Matchers
- Fixed an issue with the `hasName` and `hasAnyName` matcher when matching
inline namespaces with an enclosing namespace of the same name.

- Fixed an ordering issue with the `hasOperands` matcher occuring when setting a
binding in the first matcher and using it in the second matcher.

clang-format
------------

Expand Down
2 changes: 1 addition & 1 deletion clang/include/clang/ASTMatchers/ASTMatchers.h
Original file line number Diff line number Diff line change
Expand Up @@ -6027,7 +6027,7 @@ AST_POLYMORPHIC_MATCHER_P2(
internal::Matcher<Expr>, Matcher1, internal::Matcher<Expr>, Matcher2) {
return internal::VariadicDynCastAllOfMatcher<Stmt, NodeType>()(
anyOf(allOf(hasLHS(Matcher1), hasRHS(Matcher2)),
allOf(hasLHS(Matcher2), hasRHS(Matcher1))))
allOf(hasRHS(Matcher1), hasLHS(Matcher2))))
.matches(Node, Finder, Builder);
}

Expand Down
12 changes: 12 additions & 0 deletions clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1745,6 +1745,18 @@ TEST(MatchBinaryOperator, HasOperands) {
EXPECT_TRUE(notMatches("void x() { 0 + 1; }", HasOperands));
}

TEST(MatchBinaryOperator, HasOperandsEnsureOrdering) {
StatementMatcher HasOperandsWithBindings = binaryOperator(hasOperands(
cStyleCastExpr(has(declRefExpr(hasDeclaration(valueDecl().bind("d"))))),
declRefExpr(hasDeclaration(valueDecl(equalsBoundNode("d"))))));
EXPECT_TRUE(matches(
"int a; int b = ((int) a) + a;",
traverse(TK_IgnoreUnlessSpelledInSource, HasOperandsWithBindings)));
EXPECT_TRUE(matches(
"int a; int b = a + ((int) a);",
traverse(TK_IgnoreUnlessSpelledInSource, HasOperandsWithBindings)));
}

TEST(Matcher, BinaryOperatorTypes) {
// Integration test that verifies the AST provides all binary operators in
// a way we expect.
Expand Down

0 comments on commit f9e2a86

Please sign in to comment.