[clang-tidy] fix match for binaryOperator in ExprMutationAnalyzer for misc-const-correctness #70559
Conversation
… misc-const-correctness The `ExprMutationAnalyzer`s matcher of `binaryOperator`s that contained the variable expr, were previously narrowing the variable to be type dependent, when the `binaryOperator` should have been narrowed as dependent. The variable we are trying to find mutations for does not need to be the dependent type, the other operand of the `binaryOperator` could be dependent. Fixes llvm#57297
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-analysis @llvm/pr-subscribers-clang Author: Julian Schmidt (5chmidti) ChangesThe Fixes #57297 Full diff: https://github.com/llvm/llvm-project/pull/70559.diff 5 Files Affected:
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 5ce38ab48bf295f..bb75c9a3ce08125 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -308,6 +308,11 @@ Changes in existing checks
<clang-tidy/checks/misc/const-correctness>` check to avoid false positive when
using pointer to member function.
+- Improved :doc:`misc-const-correctness
+ <clang-tidy/checks/misc/const-correctness>` check to not warn on uses in
+ type-dependent binary operators, when the variable that is being
+ looked at, is not the dependent operand.
+
- Improved :doc:`misc-include-cleaner
<clang-tidy/checks/misc/include-cleaner>` check by adding option
`DeduplicateFindings` to output one finding per symbol occurrence, avoid
diff --git a/clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-templates.cpp b/clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-templates.cpp
index 7b5ccabdd6ef611..415bb06020b9dc3 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-templates.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-templates.cpp
@@ -20,3 +20,14 @@ void instantiate_template_cases() {
type_dependent_variables<int>();
type_dependent_variables<float>();
}
+
+namespace gh57297{
+struct Stream {};
+// Explicitly not declaring a (templated) stream operator
+// so the `<<` is a `binaryOperator` with a dependent type.
+template <typename T> void f() {
+ T t;
+ Stream stream;
+ stream << t;
+}
+} // namespace gh57297
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 2595737e8b3b143..fb9afc0646ac8cb 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -683,6 +683,9 @@ Bug Fixes to AST Handling
- Fixed a bug where RecursiveASTVisitor fails to visit the
initializer of a bitfield.
`Issue 64916 <https://github.com/llvm/llvm-project/issues/64916>`_
+- Fixed a bug where ``ExprMutationAnalyzer`` did not find a potential mutation
+ for uses in type-dependent binary operators, when the variable that is being
+ looked at, is not the dependent operand.
Miscellaneous Bug Fixes
^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Analysis/ExprMutationAnalyzer.cpp b/clang/lib/Analysis/ExprMutationAnalyzer.cpp
index f2e1beb025423cf..624a643cc60e4ba 100644
--- a/clang/lib/Analysis/ExprMutationAnalyzer.cpp
+++ b/clang/lib/Analysis/ExprMutationAnalyzer.cpp
@@ -296,9 +296,9 @@ const Stmt *ExprMutationAnalyzer::findDirectMutation(const Expr *Exp) {
// resolved and modelled as `binaryOperator` on a dependent type.
// Such instances are considered a modification, because they can modify
// in different instantiations of the template.
- binaryOperator(hasEitherOperand(
- allOf(ignoringImpCasts(canResolveToExpr(equalsNode(Exp))),
- isTypeDependent()))),
+ binaryOperator(
+ hasEitherOperand(ignoringImpCasts(canResolveToExpr(equalsNode(Exp)))),
+ isTypeDependent()),
// Within class templates and member functions the member expression might
// not be resolved. In that case, the `callExpr` is considered to be a
// modification.
diff --git a/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp b/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
index c0a398394093c48..cfa3c535ce35292 100644
--- a/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
+++ b/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
@@ -343,6 +343,17 @@ TEST(ExprMutationAnalyzerTest, UnresolvedOperator) {
EXPECT_TRUE(isMutated(Results, AST.get()));
}
+TEST(ExprMutationAnalyzerTest, DependentOperatorWithNonDependentOperand) {
+ // gh57297
+ // Explicitly not declaring a (templated) stream operator
+ // so the `<<` is a `binaryOperator` with a dependent type.
+ const auto AST = buildASTFromCode("struct Stream {}; template <typename T> "
+ "void f() { T t; Stream x; x << t;}");
+ const auto Results =
+ match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+ EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x << t"));
+}
+
// Section: expression as call argument
TEST(ExprMutationAnalyzerTest, ByValueArgument) {
|
|
CC @JonasToth for your work on |
|
The top-level Linux and Windows tests work, but inside the |
It is interesting! Generally speaking, AST of the code in Windows and Linux platform is identical with clang compiled. Probably need debuging in Windows. |
|
I found the issue, but I have not looked for a solution. Although the tested code does not directly depend on being |
|
how about adding a function call like |
- explicitly instantiate template - provide operator int() to Stream to ensure << is a BinaryOperator, not a CXXOperatorCallExpr
Yep, thanks. I went this route, but I had to define a conversion operator to compile and keep the |
|
In tests we usually use |
Done. I changed the test in |
… misc-const-correctness (llvm#70559) The `ExprMutationAnalyzer`s matcher of `binaryOperator`s that contained the variable expr, were previously narrowing the variable to be type dependent, when the `binaryOperator` should have been narrowed as dependent. The variable we are trying to find mutations for does not need to be the dependent type, the other operand of the `binaryOperator` could be dependent. Fixes llvm#57297
The
ExprMutationAnalyzers matcher ofbinaryOperatorsthat contained the variable expr, were previously narrowing the
variable to be type dependent, when the
binaryOperatorshouldhave been narrowed as dependent.
The variable we are trying to find mutations for does
not need to be the dependent type, the other operand of
the
binaryOperatorcould be dependent.Fixes #57297