Assignment and Inc/Dec operators wouldn't register as a mutation when Implicit Paren Casts were present

[Tridacnid]

Summary

Add ignoringParenImpCasts to assignment and inc/dec mutation checks in ExprMutationAnalyzer to fix clang-tidy bug PR45490.
https://bugs.llvm.org/show_bug.cgi?id=45490

[Tridacnid]

Apply clang-format patch.

[Tridacnid]

I don't think that did what I wanted. Looks like I should submit the whole change as a single patch?

[Tridacnid]

Ping

[gribozavr2]

Nice find, thank you for the fix!

[njames93]

LGTM, but I wouldn't say no to a test case using (Limit) -= 1.

[Tridacnid]

Add (Limit) -= 1 test cases to bugprone-infinite-loop tests

[aaron.ballman]

LGTM!

[Tridacnid]

Awesome. I don't have commit access, https://llvm.org/docs/Phabricator.html says I just need to ask here and someone will pick it up. Let me know if there's anything else I need to do. Thanks!

[njames93]

In D79912#2081402, @Tridacnid wrote:

Awesome. I don't have commit access, https://llvm.org/docs/Phabricator.html says I just need to ask here and someone will pick it up. Let me know if there's anything else I need to do. Thanks!

I can commit on your behalf, but I'd need to know the email address you use for github.

[Tridacnid]

[e-mail]

https://github.com/Tridacnid

Thanks!

[njames93]

In D79912#2082337, @Tridacnid wrote:

[e-mail]

https://github.com/Tridacnid

Thanks!

I've commited it on your behalf, would you be able to close the bug report.

[Tridacnid]

Bug report has been closed. I'm seeing some build failures in my inbox but eyeballing them doesn't make me think this change is related. What is the expectation for me in this scenario? Will I get an email letting me know once the build has been fixed?

[njames93]

In D79912#2083477, @Tridacnid wrote:

Bug report has been closed. I'm seeing some build failures in my inbox but eyeballing them doesn't make me think this change is related. What is the expectation for me in this scenario? Will I get an email letting me know once the build has been fixed?

If you get those emails often it means your patch was included in a bunch of patches where one caused the failed build, usually its obvious if your changes are causing the build failure. If it is your patch you will need to push a quick fix for it, or revert if it turns out to be more complicated (As you don't have commit access you can just ask someone else to do it for you). If someone else spots that a patch is causing build failures they may leave a comment in the review to point you in the right direction.
If you do decide to get commit access, its also a good idea to run ninja check-<component> before you push, with component being the part of the repo your patch touches, to reduce the chances of failures. In this case I ran check-clang and check-clang-tools, This isn't fool proof as some of the build slaves run of different platforms which have slight subtleties of how they handle certain things but will certainly increase your chance of success

[Tridacnid]

I did run both of the testing targets you mentioned, but only on Linux. I'll keep an eye on the buildbot, thanks for the info!

clang-tools-extra/test/clang-tidy/checkers/bugprone-infinite-loop.cpp

while (i < Limit) {
     Limit--;
   }
 
   while (Limit--) {
     // Not an error since 'Limit' is updated.
     i++;
   }
 
+  while ((Limit)--) {
+    // Not an error since 'Limit' is updated.
+    i++;
+  }
+
+  while ((Limit) -= 1) {
+    // Not an error since 'Limit' is updated.
+  }
+
   while (int k = Limit) {
     // Not an error since 'Limit' is updated.
     Limit--;
   }
 
+  while (int k = Limit) {
+    // Not an error since 'Limit' is updated
+    (Limit)--;
+  }
+
   while (int k = Limit--) {
     // Not an error since 'Limit' is updated.
     i++;
   }
 
   do {
     Limit--;
   } while (i < Limit);
 
   for (i = 0; i < Limit; Limit--) {
   }
+
+  for (i = 0; i < Limit; (Limit) = Limit - 1) {
+  }
+
+  for (i = 0; i < Limit; (Limit) -= 1) {
+  }
+
+  for (i = 0; i < Limit; --(Limit)) {
+  }
 }
 
 void simple_not_infinite2() {
   for (int i = 10; i-- > 0;) {
     // Not an error, since loop variable is modified in its condition part.
   }
 }
 

clang/lib/Analysis/ExprMutationAnalyzer.cpp

 const Stmt *ExprMutationAnalyzer::findDeclPointeeMutation(
     ArrayRef<ast_matchers::BoundNodes> Matches) {
   return tryEachMatch<Decl>(Matches, this,
                             &ExprMutationAnalyzer::findPointeeMutation);
 }
 
 const Stmt *ExprMutationAnalyzer::findDirectMutation(const Expr *Exp) {
   // LHS of any assignment operators.
-  const auto AsAssignmentLhs =
-      binaryOperator(isAssignmentOperator(),
-                     hasLHS(maybeEvalCommaExpr(equalsNode(Exp))));
+  const auto AsAssignmentLhs = binaryOperator(
+      isAssignmentOperator(),
+      hasLHS(maybeEvalCommaExpr(ignoringParenImpCasts(equalsNode(Exp)))));
 
   // Operand of increment/decrement operators.
   const auto AsIncDecOperand =
       unaryOperator(anyOf(hasOperatorName("++"), hasOperatorName("--")),
-                    hasUnaryOperand(maybeEvalCommaExpr(equalsNode(Exp))));
+                    hasUnaryOperand(maybeEvalCommaExpr(
+                        ignoringParenImpCasts(equalsNode(Exp)))));
 
   // Invoking non-const member function.
   // A member function is assumed to be non-const when it is unresolved.
   const auto NonConstMethod = cxxMethodDecl(unless(isConst()));
   const auto AsNonConstThis =
       expr(anyOf(cxxMemberCallExpr(callee(NonConstMethod),
                                    on(maybeEvalCommaExpr(equalsNode(Exp)))),
                  cxxOperatorCallExpr(callee(NonConstMethod),

clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp

TEST(ExprMutationAnalyzerTest, Trivial) {
   const auto Results =
       match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
   EXPECT_FALSE(isMutated(Results, AST.get()));
 }
 
 class AssignmentTest : public ::testing::TestWithParam<std::string> {};
 
 TEST_P(AssignmentTest, AssignmentModifies) {
+  {
     const std::string ModExpr = "x " + GetParam() + " 10";
     const auto AST = buildASTFromCode("void f() { int x; " + ModExpr + "; }");
     const auto Results =
         match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
     EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre(ModExpr));
   }
 
+  {
+    const std::string ModExpr = "(x) " + GetParam() + " 10";
+    const auto AST = buildASTFromCode("void f() { int x; " + ModExpr + "; }");
+    const auto Results =
+        match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+    EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre(ModExpr));
+  }
+}
+
 INSTANTIATE_TEST_CASE_P(AllAssignmentOperators, AssignmentTest,
                         Values("=", "+=", "-=", "*=", "/=", "%=", "&=", "|=",
                                "^=", "<<=", ">>="), );
 
 class IncDecTest : public ::testing::TestWithParam<std::string> {};
 
 TEST_P(IncDecTest, IncDecModifies) {
   const std::string ModExpr = GetParam();
   const auto AST = buildASTFromCode("void f() { int x; " + ModExpr + "; }");
   const auto Results =
       match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
   EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre(ModExpr));
 }
 
 INSTANTIATE_TEST_CASE_P(AllIncDecOperators, IncDecTest,
-                        Values("++x", "--x", "x++", "x--"), );
+                        Values("++x", "--x", "x++", "x--", "++(x)", "--(x)",
+                               "(x)++", "(x)--"), );
 
 TEST(ExprMutationAnalyzerTest, NonConstMemberFunc) {
   const auto AST = buildASTFromCode(
       "void f() { struct Foo { void mf(); }; Foo x; x.mf(); }");
   const auto Results =
       match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
   EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.mf()"));
 }