[analyzer] handle modification of vars inside an expr with comma operator

We should track mutation of a variable within a comma operator expression.
Current code in ExprMutationAnalyzer does not handle it.

This will handle cases like:

(a, b) ++ < == b is modified
(a, b) = c < == b is modifed


Patch by Djordje Todorovic.

Differential Revision: https://reviews.llvm.org/D58894

llvm-svn: 355605

clang/include/clang/AST/Expr.h

@@ -3403,6 +3403,9 @@ public:
   static bool isComparisonOp(Opcode Opc) { return Opc >= BO_Cmp && Opc<=BO_NE; }
   bool isComparisonOp() const { return isComparisonOp(getOpcode()); }
 
+  static bool isCommaOp(Opcode Opc) { return Opc == BO_Comma; }
+  bool isCommaOp() const { return isCommaOp(getOpcode()); }
+
   static Opcode negateComparisonOp(Opcode Opc) {
     switch (Opc) {
     default:

clang/lib/Analysis/ExprMutationAnalyzer.cpp

@@ -24,6 +24,18 @@ AST_MATCHER_P(CXXForRangeStmt, hasRangeStmt,
   return InnerMatcher.matches(*Range, Finder, Builder);
 }
 
+AST_MATCHER_P(Expr, maybeEvalCommaExpr,
+             ast_matchers::internal::Matcher<Expr>, InnerMatcher) {
+  const Expr* Result = &Node;
+  while (const auto *BOComma =
+               dyn_cast_or_null<BinaryOperator>(Result->IgnoreParens())) {
+    if (!BOComma->isCommaOp())
+      break;
+    Result = BOComma->getRHS();
+  }
+  return InnerMatcher.matches(*Result, Finder, Builder);
+}
+
 const ast_matchers::internal::VariadicDynCastAllOfMatcher<Stmt, CXXTypeidExpr>
     cxxTypeidExpr;
 
@@ -193,24 +205,28 @@ const Stmt *ExprMutationAnalyzer::findDeclPointeeMutation(
 const Stmt *ExprMutationAnalyzer::findDirectMutation(const Expr *Exp) {
   // LHS of any assignment operators.
   const auto AsAssignmentLhs =
-      binaryOperator(isAssignmentOperator(), hasLHS(equalsNode(Exp)));
+      binaryOperator(isAssignmentOperator(),
+                     hasLHS(maybeEvalCommaExpr(equalsNode(Exp))));
 
   // Operand of increment/decrement operators.
   const auto AsIncDecOperand =
       unaryOperator(anyOf(hasOperatorName("++"), hasOperatorName("--")),
-                    hasUnaryOperand(equalsNode(Exp)));
+                    hasUnaryOperand(maybeEvalCommaExpr(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(equalsNode(Exp))),
+      expr(anyOf(cxxMemberCallExpr(callee(NonConstMethod),
+                                   on(maybeEvalCommaExpr(equalsNode(Exp)))),
                  cxxOperatorCallExpr(callee(NonConstMethod),
-                                     hasArgument(0, equalsNode(Exp))),
+                                     hasArgument(0,
+                                                 maybeEvalCommaExpr(equalsNode(Exp)))),
                  callExpr(callee(expr(anyOf(
-                     unresolvedMemberExpr(hasObjectExpression(equalsNode(Exp))),
+                     unresolvedMemberExpr(
+                       hasObjectExpression(maybeEvalCommaExpr(equalsNode(Exp)))),
                      cxxDependentScopeMemberExpr(
-                         hasObjectExpression(equalsNode(Exp)))))))));
+                         hasObjectExpression(maybeEvalCommaExpr(equalsNode(Exp))))))))));
 
   // Taking address of 'Exp'.
   // We're assuming 'Exp' is mutated as soon as its address is taken, though in
@@ -220,10 +236,11 @@ const Stmt *ExprMutationAnalyzer::findDirectMutation(const Expr *Exp) {
       unaryOperator(hasOperatorName("&"),
                     // A NoOp implicit cast is adding const.
                     unless(hasParent(implicitCastExpr(hasCastKind(CK_NoOp)))),
-                    hasUnaryOperand(equalsNode(Exp)));
+                    hasUnaryOperand(maybeEvalCommaExpr(equalsNode(Exp))));
   const auto AsPointerFromArrayDecay =
       castExpr(hasCastKind(CK_ArrayToPointerDecay),
-               unless(hasParent(arraySubscriptExpr())), has(equalsNode(Exp)));
+               unless(hasParent(arraySubscriptExpr())),
+               has(maybeEvalCommaExpr(equalsNode(Exp))));
   // Treat calling `operator->()` of move-only classes as taking address.
   // These are typically smart pointers with unique ownership so we treat
   // mutation of pointee as mutation of the smart pointer itself.
@@ -231,7 +248,8 @@ const Stmt *ExprMutationAnalyzer::findDirectMutation(const Expr *Exp) {
       cxxOperatorCallExpr(hasOverloadedOperatorName("->"),
                           callee(cxxMethodDecl(ofClass(isMoveOnly()),
                                                returns(nonConstPointerType()))),
-                          argumentCountIs(1), hasArgument(0, equalsNode(Exp)));
+                          argumentCountIs(1),
+                          hasArgument(0, maybeEvalCommaExpr(equalsNode(Exp))));
 
   // Used as non-const-ref argument when calling a function.
   // An argument is assumed to be non-const-ref when the function is unresolved.
@@ -239,7 +257,8 @@ const Stmt *ExprMutationAnalyzer::findDirectMutation(const Expr *Exp) {
   // findFunctionArgMutation which has additional smarts for handling forwarding
   // references.
   const auto NonConstRefParam = forEachArgumentWithParam(
-      equalsNode(Exp), parmVarDecl(hasType(nonConstReferenceType())));
+      maybeEvalCommaExpr(equalsNode(Exp)),
+      parmVarDecl(hasType(nonConstReferenceType())));
   const auto NotInstantiated = unless(hasDeclaration(isInstantiated()));
   const auto AsNonConstRefArg = anyOf(
       callExpr(NonConstRefParam, NotInstantiated),
@@ -247,8 +266,8 @@ const Stmt *ExprMutationAnalyzer::findDirectMutation(const Expr *Exp) {
       callExpr(callee(expr(anyOf(unresolvedLookupExpr(), unresolvedMemberExpr(),
                                  cxxDependentScopeMemberExpr(),
                                  hasType(templateTypeParmType())))),
-               hasAnyArgument(equalsNode(Exp))),
+               hasAnyArgument(maybeEvalCommaExpr(equalsNode(Exp)))),
-      cxxUnresolvedConstructExpr(hasAnyArgument(equalsNode(Exp))));
+      cxxUnresolvedConstructExpr(hasAnyArgument(maybeEvalCommaExpr(equalsNode(Exp)))));
 
   // Captured by a lambda by reference.
   // If we're initializing a capture with 'Exp' directly then we're initializing
@@ -261,7 +280,8 @@ const Stmt *ExprMutationAnalyzer::findDirectMutation(const Expr *Exp) {
   // For returning by value there will be an ImplicitCastExpr <LValueToRValue>.
   // For returning by const-ref there will be an ImplicitCastExpr <NoOp> (for
   // adding const.)
-  const auto AsNonConstRefReturn = returnStmt(hasReturnValue(equalsNode(Exp)));
+  const auto AsNonConstRefReturn = returnStmt(hasReturnValue(
+                                                maybeEvalCommaExpr(equalsNode(Exp))));
 
   const auto Matches =
       match(findAll(stmt(anyOf(AsAssignmentLhs, AsIncDecOperand, AsNonConstThis,

clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp

@@ -881,6 +881,137 @@ TEST(ExprMutationAnalyzerTest, CastToConstRef) {
   EXPECT_FALSE(isMutated(Results, AST.get()));
 }
 
+TEST(ExprMutationAnalyzerTest, CommaExprWithAnAssigment) {
+  const auto AST =
+      buildASTFromCodeWithArgs("void f() { int x; int y; (x, y) = 5; }",
+                               {"-Wno-unused-value"});
+  const auto Results =
+      match(withEnclosingCompound(declRefTo("y")), AST->getASTContext());
+  EXPECT_TRUE(isMutated(Results, AST.get()));
+}
+
+TEST(ExprMutationAnalyzerTest, CommaExprWithDecOp) {
+  const auto AST =
+      buildASTFromCodeWithArgs("void f() { int x; int y; (x, y)++; }",
+                               {"-Wno-unused-value"});
+  const auto Results =
+      match(withEnclosingCompound(declRefTo("y")), AST->getASTContext());
+  EXPECT_TRUE(isMutated(Results, AST.get()));
+}
+
+TEST(ExprMutationAnalyzerTest, CommaExprWithNonConstMemberCall) {
+  const auto AST =
+      buildASTFromCodeWithArgs("class A { public: int mem; void f() { mem ++; } };"
+                               "void fn() { A o1, o2; (o1, o2).f(); }",
+                               {"-Wno-unused-value"});
+  const auto Results =
+      match(withEnclosingCompound(declRefTo("o2")), AST->getASTContext());
+  EXPECT_TRUE(isMutated(Results, AST.get()));
+}
+
+TEST(ExprMutationAnalyzerTest, CommaExprWithConstMemberCall) {
+  const auto AST =
+      buildASTFromCodeWithArgs("class A { public: int mem; void f() const  { } };"
+                               "void fn() { A o1, o2; (o1, o2).f(); }",
+                               {"-Wno-unused-value"});
+  const auto Results =
+      match(withEnclosingCompound(declRefTo("o2")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+}
+
+TEST(ExprMutationAnalyzerTest, CommaExprWithCallExpr) {
+  const auto AST =
+      buildASTFromCodeWithArgs("class A { public: int mem; void f(A &O1) {} };"
+                               "void fn() { A o1, o2; o2.f((o2, o1)); }",
+                               {"-Wno-unused-value"});
+  const auto Results =
+      match(withEnclosingCompound(declRefTo("o1")), AST->getASTContext());
+  EXPECT_TRUE(isMutated(Results, AST.get()));
+}
+
+TEST(ExprMutationAnalyzerTest, CommaExprWithCallUnresolved) {
+  auto AST = buildASTFromCodeWithArgs(
+      "template <class T> struct S;"
+      "template <class T> void f() { S<T> s; int x, y; s.mf((y, x)); }",
+      {"-fno-delayed-template-parsing -Wno-unused-value"});
+  auto Results =
+      match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_TRUE(isMutated(Results, AST.get()));
+
+  AST = buildASTFromCodeWithArgs(
+      "template <class T> void f(T t) { int x, y; g(t, (y, x)); }",
+      {"-fno-delayed-template-parsing -Wno-unused-value"});
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_TRUE(isMutated(Results, AST.get()));
+}
+
+TEST(ExprMutationAnalyzerTest, CommaExprParmRef) {
+  const auto AST =
+      buildASTFromCodeWithArgs("class A { public: int mem;};"
+                               "extern void fn(A &o1);"
+                               "void fn2 () { A o1, o2; fn((o2, o1)); } ",
+                               {"-Wno-unused-value"});
+  const auto Results =
+      match(withEnclosingCompound(declRefTo("o1")), AST->getASTContext());
+  EXPECT_TRUE(isMutated(Results, AST.get()));
+}
+
+TEST(ExprMutationAnalyzerTest, CommaExprWithAmpersandOp) {
+  const auto AST =
+      buildASTFromCodeWithArgs("class A { public: int mem;};"
+                               "void fn () { A o1, o2;"
+                               "void *addr = &(o2, o1); } ",
+                               {"-Wno-unused-value"});
+  const auto Results =
+      match(withEnclosingCompound(declRefTo("o1")), AST->getASTContext());
+  EXPECT_TRUE(isMutated(Results, AST.get()));
+}
+
+TEST(ExprMutationAnalyzerTest, CommaExprAsReturnAsValue) {
+  auto AST = buildASTFromCodeWithArgs("int f() { int x, y; return (x, y); }",
+                                      {"-Wno-unused-value"});
+  auto Results =
+      match(withEnclosingCompound(declRefTo("y")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+}
+
+TEST(ExprMutationAnalyzerTest, CommaEpxrAsReturnAsNonConstRef) {
+  const auto AST =
+      buildASTFromCodeWithArgs("int& f() { int x, y; return (y, x); }",
+                               {"-Wno-unused-value"});
+  const auto Results =
+      match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_TRUE(isMutated(Results, AST.get()));
+}
+
+TEST(ExprMutationAnalyzerTest, CommaExprAsArrayToPointerDecay) {
+  const auto AST =
+      buildASTFromCodeWithArgs("void g(int*); "
+                               "void f() { int x[2], y[2]; g((y, x)); }",
+                               {"-Wno-unused-value"});
+  const auto Results =
+      match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_TRUE(isMutated(Results, AST.get()));
+}
+
+TEST(ExprMutationAnalyzerTest, CommaExprAsUniquePtr) {
+  const std::string UniquePtrDef =
+      "template <class T> struct UniquePtr {"
+      "  UniquePtr();"
+      "  UniquePtr(const UniquePtr&) = delete;"
+      "  T& operator*() const;"
+      "  T* operator->() const;"
+      "};";
+  const auto AST = buildASTFromCodeWithArgs(
+      UniquePtrDef + "template <class T> void f() "
+                     "{ UniquePtr<T> x; UniquePtr<T> y;"
+                     " (y, x)->mf(); }",
+      {"-fno-delayed-template-parsing -Wno-unused-value"});
+  const auto Results =
+      match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_TRUE(isMutated(Results, AST.get()));
+}
+
 TEST(ExprMutationAnalyzerTest, LambdaDefaultCaptureByValue) {
   const auto AST = buildASTFromCode("void f() { int x; [=]() { x; }; }");
   const auto Results =