hasAnyArgument() spuriously ignores match #75754

xb8g2pw0 · 2023-12-17T22:57:48Z

hasAnyArgument() spuriously ignores a match with the program:

void foo (void *, void *);

struct S {
};

void f() {
   S s;
   foo(&s, &s);
}

and the query:

m callExpr(hasAnyArgument(hasType(asString("S *"))))
0 matches.

However, the query:

m callExpr(hasArgument(0,hasType(asString("S *"))))

returns the expected result:

...clang_ast_bug_2.cpp:10:2: note: "root" binds here
 10 |         foo(&s, &s);
      |         ^~~~~~~~~~~
1 match.

I am using clang-query 17.0.2 built, I believe, by Mozilla.

The text was updated successfully, but these errors were encountered:

danix800 · 2023-12-18T01:26:56Z

hasArgument() ignores any implicit casts & parenthesis around the argument via an extra Expr::IgnoreParenImpCasts(), which I think is not necessary, while hasAnyArgument() does not ignore anything.

So if you want hasAnyArgument() to work on this example, use m callExpr(hasAnyArgument( ignoringParenImpCasts(hasType(asString("S *"))))), and an extra ignoringParenImpCasts():

m callExpr(hasAnyArgument(ignoringParenImpCasts(hasType(asString("S *")))))

Match #1:

...test.cpp:8:4: note: "root" binds here
   foo(&s, &s);
   ^~~~~~~~~~~
1 match.

hasArgument() might need to be fixed but I'm afraid that would break a lot of clients.

llvmbot · 2023-12-18T02:41:52Z

@llvm/issue-subscribers-clang-frontend

Author: None (xb8g2pw0)

`hasAnyArgument()` spuriously ignores a match with the program:

void foo (void *, void *);

struct S {
};

void f() {
   S s;
   foo(&amp;s, &amp;s);
}

and the query:

m callExpr(hasAnyArgument(hasType(asString("S *"))))
0 matches.

However, the query:

m callExpr(hasArgument(0,hasType(asString("S *"))))

returns the expected result:

...clang_ast_bug_2.cpp:10:2: note: "root" binds here
 10 |         foo(&amp;s, &amp;s);
      |         ^~~~~~~~~~~
1 match.

I am using clang-query 17.0.2 built, I believe, by Mozilla.

llvmbot · 2023-12-18T18:14:39Z

@llvm/issue-subscribers-clang-query

Author: None (xb8g2pw0)

`hasAnyArgument()` spuriously ignores a match with the program:

void foo (void *, void *);

struct S {
};

void f() {
   S s;
   foo(&amp;s, &amp;s);
}

and the query:

m callExpr(hasAnyArgument(hasType(asString("S *"))))
0 matches.

However, the query:

m callExpr(hasArgument(0,hasType(asString("S *"))))

returns the expected result:

...clang_ast_bug_2.cpp:10:2: note: "root" binds here
 10 |         foo(&amp;s, &amp;s);
      |         ^~~~~~~~~~~
1 match.

I am using clang-query 17.0.2 built, I believe, by Mozilla.

xb8g2pw0 · 2023-12-18T19:35:09Z

Thanks for the info. Probably this should be considered a documentation bug? https://clang.llvm.org/docs/LibASTMatchersReference.html does not describe this difference between hasArgument() and hasAnyArgument().

AaronBallman · 2024-01-11T19:18:13Z

I think it's a functionality bug and not a documentation bug. The intent behind hasArgument and hasAnyArgument is that hasArgument cares about the position of the argument and hasAnyArgument does not. I don't think it should have different behavior regarding implicit AST nodes.

llvmbot · 2024-01-11T19:22:08Z

Hi!

This issue may be a good introductory issue for people new to working on LLVM. If you would like to work on this issue, your first steps are:

In the comments of the issue, request for it to be assigned to you.
Fix the issue locally.
Run the test suite locally. Remember that the subdirectories under test/ create fine-grained testing targets, so you can e.g. use make check-clang-ast to only run Clang's AST tests.
Create a Git commit.
Run git clang-format HEAD~1 to format your changes.
Open a pull request to the upstream repository on GitHub. Detailed instructions can be found in GitHub's documentation.

If you have any further questions about this issue, don't hesitate to ask via a comment in the thread below.

llvmbot · 2024-01-11T19:22:08Z

@llvm/issue-subscribers-good-first-issue

Author: None (xb8g2pw0)

`hasAnyArgument()` spuriously ignores a match with the program:

void foo (void *, void *);

struct S {
};

void f() {
   S s;
   foo(&amp;s, &amp;s);
}

and the query:

m callExpr(hasAnyArgument(hasType(asString("S *"))))
0 matches.

However, the query:

m callExpr(hasArgument(0,hasType(asString("S *"))))

returns the expected result:

...clang_ast_bug_2.cpp:10:2: note: "root" binds here
 10 |         foo(&amp;s, &amp;s);
      |         ^~~~~~~~~~~
1 match.

I am using clang-query 17.0.2 built, I believe, by Mozilla.

AaronBallman · 2024-01-11T19:25:44Z

I'm marking this as a good first issue because I think the correct behavior is to not ignore implicit AST nodes in hasArgument. We have the TK_IgnoreUnlessSpelledInSource traversal kind for when the user wants to silently skip implicit nodes.

Note, this may cause some fallout that makes this not quite a good first issue because we may have clang-tidy checks that rely on ignoring the implicit nodes. So the fix may be somewhat more involved due to needing to also repair problems with clang-tidy checks.

CC @PiotrZSL @carlosgalvezp @njames93 @LegalizeAdulthood @sam-mccall for extra opinions and thoughts.

LegalizeAdulthood · 2024-01-11T20:54:09Z

I say go for it. If anyone fixing this gets weird fallout in clang-tidy, we can help out.

Rajveer100 · 2024-01-13T10:54:44Z

@AaronBallman @LegalizeAdulthood
The issue looks clear to me, could you provide a comprehensive insight from your end to get me going?

11happy · 2024-01-28T11:48:21Z

so I removed this additional condition *Arg->IgnoreParenImpCasts() and ran tests , It broke 92 tests.

********************
********************
Failed Tests (92):
  Clang :: Analysis/osobjectcstylecastchecker_test.cpp
  Clang Tools :: clang-tidy/checkers/abseil/redundant-strcat-calls.cpp
  Clang Tools :: clang-tidy/checkers/abseil/string-find-startswith.cpp
  Clang Tools :: clang-tidy/checkers/bugprone/dangling-handle.cpp
  Clang Tools :: clang-tidy/checkers/bugprone/signal-handler-minimal.c
  Clang Tools :: clang-tidy/checkers/bugprone/signal-handler-posix.c
  Clang Tools :: clang-tidy/checkers/bugprone/signal-handler.c
  Clang Tools :: clang-tidy/checkers/bugprone/signal-handler.cpp
  Clang Tools :: clang-tidy/checkers/bugprone/signed-char-misuse.cpp
  Clang Tools :: clang-tidy/checkers/bugprone/string-integer-assignment.cpp
  Clang Tools :: clang-tidy/checkers/bugprone/undefined-memory-manipulation.cpp
  Clang Tools :: clang-tidy/checkers/bugprone/use-after-move.cpp
  Clang Tools :: clang-tidy/checkers/cert/env33-c.c
  Clang Tools :: clang-tidy/checkers/cert/oop57-cpp.cpp
  Clang Tools :: clang-tidy/checkers/cppcoreguidelines/pro-bounds-constant-array-index-gslheader.cpp
  Clang Tools :: clang-tidy/checkers/cppcoreguidelines/pro-bounds-constant-array-index.cpp
  Clang Tools :: clang-tidy/checkers/cppcoreguidelines/rvalue-reference-param-not-moved.cpp
  Clang Tools :: clang-tidy/checkers/cppcoreguidelines/slicing.cpp
  Clang Tools :: clang-tidy/checkers/llvm/prefer-isa-or-dyn-cast-in-conditionals.cpp
  Clang Tools :: clang-tidy/checkers/modernize/make-shared.cpp
  Clang Tools :: clang-tidy/checkers/modernize/make-unique-default-init.cpp
  Clang Tools :: clang-tidy/checkers/modernize/make-unique.cpp
  Clang Tools :: clang-tidy/checkers/modernize/use-equals-default-copy.cpp
  Clang Tools :: clang-tidy/checkers/performance/type-promotion-in-math-fn.cpp
  Clang Tools :: clang-tidy/checkers/performance/unnecessary-copy-initialization.cpp
  Clang Tools :: clang-tidy/checkers/readability/redundant-string-cstr.cpp
  Clang-Unit :: ASTMatchers/./ASTMatchersTests/ASTMatchersTests/ASTMatchersTest/AllOf/0
  Clang-Unit :: ASTMatchers/./ASTMatchersTests/ASTMatchersTests/ASTMatchersTest/AllOf/1
  Clang-Unit :: ASTMatchers/./ASTMatchersTests/ASTMatchersTests/ASTMatchersTest/AllOf/10
  Clang-Unit :: ASTMatchers/./ASTMatchersTests/ASTMatchersTests/ASTMatchersTest/AllOf/11
  Clang-Unit :: ASTMatchers/./ASTMatchersTests/ASTMatchersTests/ASTMatchersTest/AllOf/12
  Clang-Unit :: ASTMatchers/./ASTMatchersTests/ASTMatchersTests/ASTMatchersTest/AllOf/13
  Clang-Unit :: ASTMatchers/./ASTMatchersTests/ASTMatchersTests/ASTMatchersTest/AllOf/2
  Clang-Unit :: ASTMatchers/./ASTMatchersTests/ASTMatchersTests/ASTMatchersTest/AllOf/3
  Clang-Unit :: ASTMatchers/./ASTMatchersTests/ASTMatchersTests/ASTMatchersTest/AllOf/4
  Clang-Unit :: ASTMatchers/./ASTMatchersTests/ASTMatchersTests/ASTMatchersTest/AllOf/5
  Clang-Unit :: ASTMatchers/./ASTMatchersTests/ASTMatchersTests/ASTMatchersTest/AllOf/6
  Clang-Unit :: ASTMatchers/./ASTMatchersTests/ASTMatchersTests/ASTMatchersTest/AllOf/7
  Clang-Unit :: ASTMatchers/./ASTMatchersTests/ASTMatchersTests/ASTMatchersTest/AllOf/8
  Clang-Unit :: ASTMatchers/./ASTMatchersTests/ASTMatchersTests/ASTMatchersTest/AllOf/9
  Clang-Unit :: ASTMatchers/./ASTMatchersTests/ASTMatchersTests/ASTMatchersTest/HasArgument_CXXConstructorDecl/10
  Clang-Unit :: ASTMatchers/./ASTMatchersTests/ASTMatchersTests/ASTMatchersTest/HasArgument_CXXConstructorDecl/11
  Clang-Unit :: ASTMatchers/./ASTMatchersTests/ASTMatchersTests/ASTMatchersTest/HasArgument_CXXConstructorDecl/12
  Clang-Unit :: ASTMatchers/./ASTMatchersTests/ASTMatchersTests/ASTMatchersTest/HasArgument_CXXConstructorDecl/13
  Clang-Unit :: ASTMatchers/./ASTMatchersTests/ASTMatchersTests/ASTMatchersTest/HasArgument_CXXConstructorDecl/4
  Clang-Unit :: ASTMatchers/./ASTMatchersTests/ASTMatchersTests/ASTMatchersTest/HasArgument_CXXConstructorDecl/5
  Clang-Unit :: ASTMatchers/./ASTMatchersTests/ASTMatchersTests/ASTMatchersTest/HasArgument_CXXConstructorDecl/6
  Clang-Unit :: ASTMatchers/./ASTMatchersTests/ASTMatchersTests/ASTMatchersTest/HasArgument_CXXConstructorDecl/7
  Clang-Unit :: ASTMatchers/./ASTMatchersTests/ASTMatchersTests/ASTMatchersTest/HasArgument_CXXConstructorDecl/8
  Clang-Unit :: ASTMatchers/./ASTMatchersTests/ASTMatchersTests/ASTMatchersTest/HasArgument_CXXConstructorDecl/9
  Clang-Unit :: ASTMatchers/./ASTMatchersTests/ASTMatchersTests/ASTMatchersTest/IsInteger_MatchesIntegers/0
  Clang-Unit :: ASTMatchers/./ASTMatchersTests/ASTMatchersTests/ASTMatchersTest/IsInteger_MatchesIntegers/1
  Clang-Unit :: ASTMatchers/./ASTMatchersTests/ASTMatchersTests/ASTMatchersTest/IsInteger_MatchesIntegers/10
  Clang-Unit :: ASTMatchers/./ASTMatchersTests/ASTMatchersTests/ASTMatchersTest/IsInteger_MatchesIntegers/11
  Clang-Unit :: ASTMatchers/./ASTMatchersTests/ASTMatchersTests/ASTMatchersTest/IsInteger_MatchesIntegers/12
  Clang-Unit :: ASTMatchers/./ASTMatchersTests/ASTMatchersTests/ASTMatchersTest/IsInteger_MatchesIntegers/13
  Clang-Unit :: ASTMatchers/./ASTMatchersTests/ASTMatchersTests/ASTMatchersTest/IsInteger_MatchesIntegers/2
  Clang-Unit :: ASTMatchers/./ASTMatchersTests/ASTMatchersTests/ASTMatchersTest/IsInteger_MatchesIntegers/3
  Clang-Unit :: ASTMatchers/./ASTMatchersTests/ASTMatchersTests/ASTMatchersTest/IsInteger_MatchesIntegers/4
  Clang-Unit :: ASTMatchers/./ASTMatchersTests/ASTMatchersTests/ASTMatchersTest/IsInteger_MatchesIntegers/5
  Clang-Unit :: ASTMatchers/./ASTMatchersTests/ASTMatchersTests/ASTMatchersTest/IsInteger_MatchesIntegers/6
  Clang-Unit :: ASTMatchers/./ASTMatchersTests/ASTMatchersTests/ASTMatchersTest/IsInteger_MatchesIntegers/7
  Clang-Unit :: ASTMatchers/./ASTMatchersTests/ASTMatchersTests/ASTMatchersTest/IsInteger_MatchesIntegers/8
  Clang-Unit :: ASTMatchers/./ASTMatchersTests/ASTMatchersTests/ASTMatchersTest/IsInteger_MatchesIntegers/9
  Clang-Unit :: ASTMatchers/./ASTMatchersTests/GtestAssertTest/ShouldMatchAssert
  Clang-Unit :: ASTMatchers/./ASTMatchersTests/GtestExpectCallTest/CallShouldMatchExpectCallWithParams1
  Clang-Unit :: ASTMatchers/./ASTMatchersTests/GtestExpectCallTest/CallShouldMatchExpectCallWithParams2
  Clang-Unit :: ASTMatchers/./ASTMatchersTests/GtestExpectTest/ShouldMatchExpect
  Clang-Unit :: ASTMatchers/./ASTMatchersTests/GtestOnCallTest/CallShouldMatchOnCallWithParams1
  Clang-Unit :: ASTMatchers/./ASTMatchersTests/GtestOnCallTest/CallShouldMatchOnCallWithParams2
  Clang-Unit :: ASTMatchers/./ASTMatchersTests/Matcher/Argument
  Clang-Unit :: ASTMatchers/./ASTMatchersTests/Matcher/IgnoresElidableConstructors
  Clang-Unit :: ASTMatchers/./ASTMatchersTests/Traversal/traverseUnlessSpelledInSource
  Clang-Unit :: Analysis/./ClangAnalysisTests/ExprMutationAnalyzerTest/UniquePtr
  Clang-Unit :: Analysis/FlowSensitive/./ClangAnalysisFlowSensitiveTests/UncheckedOptionalUseTestInst/UncheckedOptionalAccessTest/ValueOrComparisonIntegers/absl
  Clang-Unit :: Analysis/FlowSensitive/./ClangAnalysisFlowSensitiveTests/UncheckedOptionalUseTestInst/UncheckedOptionalAccessTest/ValueOrComparisonIntegers/base
  Clang-Unit :: Analysis/FlowSensitive/./ClangAnalysisFlowSensitiveTests/UncheckedOptionalUseTestInst/UncheckedOptionalAccessTest/ValueOrComparisonIntegers/std
  Clang-Unit :: Analysis/FlowSensitive/./ClangAnalysisFlowSensitiveTests/UncheckedOptionalUseTestInst/UncheckedOptionalAccessTest/ValueOrComparisonPointerToOptional/absl
  Clang-Unit :: Analysis/FlowSensitive/./ClangAnalysisFlowSensitiveTests/UncheckedOptionalUseTestInst/UncheckedOptionalAccessTest/ValueOrComparisonPointerToOptional/base
  Clang-Unit :: Analysis/FlowSensitive/./ClangAnalysisFlowSensitiveTests/UncheckedOptionalUseTestInst/UncheckedOptionalAccessTest/ValueOrComparisonPointerToOptional/std
  Clang-Unit :: Analysis/FlowSensitive/./ClangAnalysisFlowSensitiveTests/UncheckedOptionalUseTestInst/UncheckedOptionalAccessTest/ValueOrComparisonPointers/absl
  Clang-Unit :: Analysis/FlowSensitive/./ClangAnalysisFlowSensitiveTests/UncheckedOptionalUseTestInst/UncheckedOptionalAccessTest/ValueOrComparisonPointers/base
  Clang-Unit :: Analysis/FlowSensitive/./ClangAnalysisFlowSensitiveTests/UncheckedOptionalUseTestInst/UncheckedOptionalAccessTest/ValueOrComparisonPointers/std
  Clang-Unit :: Tooling/./ToolingTests/StencilTest/CatOfInvalidRangeFails
  Clang-Unit :: Tooling/./ToolingTests/TransformerTest/FunctionMacro
  Clang-Unit :: Tooling/./ToolingTests/TransformerTest/IdentityMacro
  Clang-Unit :: Tooling/./ToolingTests/TransformerTest/MacroArg
  Clang-Unit :: Tooling/./ToolingTests/TransformerTest/MacroArgInMacroDef
  Clang-Unit :: Tooling/./ToolingTests/TransformerTest/OrderedRuleUnrelated
  Clang-Unit :: Tooling/./ToolingTests/TransformerTest/StrlenSize
  Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/ConstReferenceDeclRefExprsTest/RefVar
  Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/ConstReferenceDeclRefExprsTest/ValueVar


Testing Time: 645.98s

Total Discovered Tests: 96385
  Skipped          :   333 (0.35%)
  Unsupported      : 30803 (31.96%)
  Passed           : 65077 (67.52%)
  Expectedly Failed:    80 (0.08%)
  Failed           :    92 (0.10%)

vinayakdsci · 2024-03-03T01:34:22Z

Hello!
If I understand correctly, the task of fixing this issue would require simply the removal of the IgnoreParenImpCasts() from the hasArgument matcher, to make its behavior consistent with hasAnyArgument in this case and then fixing the tests that break consequently?

AaronBallman · 2024-03-04T20:21:45Z

Hello! If I understand correctly, the task of fixing this issue would require simply the removal of the IgnoreParenImpCasts() from the hasArgument matcher, to make its behavior consistent with hasAnyArgument in this case and then fixing the tests that break consequently?

That's correct!

PiotrZSL · 2024-03-04T21:05:56Z

Most probably better will be to change all current usages of hasAnyArgument(x) into hasAnyArgument(ignoringParenImpCasts(x))

komalverma04 · 2024-03-28T19:39:53Z

@AaronBallman I would like to take up this issue, please assign it to me.

komalverma04 · 2024-03-30T15:34:51Z

Hi @AaronBallman, I have run LLVM regression tests. And ninja check-clang command is running.Please help me with which tests to run.

AaronBallman · 2024-04-01T13:14:15Z

Hi @AaronBallman, I have run LLVM regression tests. And ninja check-clang command is running.Please help me with which tests to run.

ninja check-clang will run all of Clang's tests automatically, but you may also want to run ninja check-clang-tools as well because of how much AST matchers are used by clang-tidy.

github-actions bot added the new issue label Dec 17, 2023

EugeneZelenko added clang:frontend Language frontend issues, e.g. anything involving "Sema" and removed new issue labels Dec 18, 2023

shafik added the clang-query label Dec 18, 2023

AaronBallman added the confirmed Verified by a second party label Jan 11, 2024

AaronBallman added the good first issue https://github.com/llvm/llvm-project/contribute label Jan 11, 2024

AaronBallman assigned komalverma04 Mar 28, 2024

komalverma04 mentioned this issue Apr 1, 2024

Add ignoringParenImpCasts in hasAnyArgument fix#75754 #87268

Closed

This was referenced Apr 20, 2024

Add ignoring paren imp casts in has any argument #89509

Open

Add ignoringParenImpCasts in arguments of hasArgument #89553

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

hasAnyArgument() spuriously ignores match #75754

hasAnyArgument() spuriously ignores match #75754

xb8g2pw0 commented Dec 17, 2023

danix800 commented Dec 18, 2023

llvmbot commented Dec 18, 2023

llvmbot commented Dec 18, 2023

xb8g2pw0 commented Dec 18, 2023

AaronBallman commented Jan 11, 2024

llvmbot commented Jan 11, 2024

llvmbot commented Jan 11, 2024

AaronBallman commented Jan 11, 2024

LegalizeAdulthood commented Jan 11, 2024

Rajveer100 commented Jan 13, 2024

11happy commented Jan 28, 2024

vinayakdsci commented Mar 3, 2024

AaronBallman commented Mar 4, 2024

PiotrZSL commented Mar 4, 2024

komalverma04 commented Mar 28, 2024

komalverma04 commented Mar 30, 2024 •

edited

Loading

AaronBallman commented Apr 1, 2024

hasAnyArgument() spuriously ignores match #75754

hasAnyArgument() spuriously ignores match #75754

Comments

xb8g2pw0 commented Dec 17, 2023

danix800 commented Dec 18, 2023

llvmbot commented Dec 18, 2023

llvmbot commented Dec 18, 2023

xb8g2pw0 commented Dec 18, 2023

AaronBallman commented Jan 11, 2024

llvmbot commented Jan 11, 2024

llvmbot commented Jan 11, 2024

AaronBallman commented Jan 11, 2024

LegalizeAdulthood commented Jan 11, 2024

Rajveer100 commented Jan 13, 2024

11happy commented Jan 28, 2024

vinayakdsci commented Mar 3, 2024

AaronBallman commented Mar 4, 2024

PiotrZSL commented Mar 4, 2024

komalverma04 commented Mar 28, 2024

komalverma04 commented Mar 30, 2024 • edited Loading

AaronBallman commented Apr 1, 2024

komalverma04 commented Mar 30, 2024 •

edited

Loading