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

[djtodoro]

Summary

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

[lebedev.ri]

Thank you for working on this!

Is there any way to model this more generically?
I.e don't duplicate every naive matcher (ignoring possible presence of , op) with an variant that does not ignore ,.
E.g. will this handle (a,b)+=1 ?
What about (a,b).callNonConstMethod(), (a,b).callConstMethod() ?

[djtodoro]

@lebedev.ri Thanks for your comment!

Is there any way to model this more generically?
I.e don't duplicate every naive matcher (ignoring possible presence of , op) with an variant that does not ignore ,.
E.g. will this handle (a,b)+=1 ?

Hmm, I am not sure if there is a place for that, since this is very specific to comma operators. Any suggestions ?
It handles the situation ((a,b)+=1).

What about (a,b).callNonConstMethod(), (a,b).callConstMethod() ?

This needs to be extended to support function/method calls. I guess ExprMutationAnalyzer::findFunctionArgMutation requires an improvement.

[lebedev.ri]

In D58894#1416691, @djtodoro wrote:

@lebedev.ri Thanks for your comment!

Is there any way to model this more generically?
I.e don't duplicate every naive matcher (ignoring possible presence of , op) with an variant that does not ignore ,.
E.g. will this handle (a,b)+=1 ?

Hmm, I am not sure if there is a place for that, since this is very specific to comma operators. Any suggestions ?
It handles the situation ((a,b)+=1).

What about (a,b).callNonConstMethod(), (a,b).callConstMethod() ?

This needs to be extended to support function/method calls. I guess ExprMutationAnalyzer::findFunctionArgMutation requires an improvement.

Well, given

// LHS of any assignment operators.
const auto AsAssignmentLhs =
    binaryOperator(isAssignmentOperator(), hasLHS(equalsNode(Exp)));

The problem is that hasLHS() may get , op.
In isAssigmentWithCommma(), you check if that is , op, and if so, return it's right hand.
And this duplication is the problem as far as i can see.

Now, i don't know the final solution, but have you considered adding something like:

AST_MATCHER_P(Expr, skipCommaOps,
              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 Result;
}

and then simply changing

// LHS of any assignment operators.
const auto AsAssignmentLhs =
    binaryOperator(isAssignmentOperator(), hasLHS(equalsNode(Exp)));

to

// LHS of any assignment operators.
const auto AsAssignmentLhs =
    binaryOperator(isAssignmentOperator(), hasLHS(skipCommaOps(equalsNode(Exp))));

?

[djtodoro]

@lebedev.ri I agree, thank you! I needed to be more precise in my previous reply, sorry for that. I thought it will be (somehow) overhead if I change existing, very basic, matchers.

I already implemented a static function that skips comma operands, and extended this to support member calls, functions, etc.
But, implementing it as a new matcher sounds like better idea.

Thanks!

[lebedev.ri]

In D58894#1418251, @djtodoro wrote:

@lebedev.ri I agree, thank you! I needed to be more precise in my previous reply, sorry for that. I thought it will be (somehow) overhead if I change existing, very basic, matchers.

I indeed don't think the existing matchers should be changed to ignore these , ops (or implicit casts, like some issue reports propose).

I already implemented a static function that skips comma operands, and extended this to support member calls, functions, etc.

But, implementing it as a new matcher sounds like better idea.

Yes. I think this matcher will be very baseline, and can just be added where needed
(with appropriate test coverage, of course), without matcher duplication like in this current diff.

Thanks!

Thanks for working on this.

[djtodoro]

-add AST matcher that skips operands in comma expression
-add unit tests for extended support

[djtodoro]

@lebedev.ri I agree, thank you! I needed to be more precise in my previous reply, sorry for that. I thought it will be (somehow) overhead if I change existing, very basic, matchers.

I indeed don't think the existing matchers should be changed to ignore these , ops (or implicit casts, like some issue reports propose).

I already implemented a static function that skips comma operands, and extended this to support member calls, functions, etc.
But, implementing it as a new matcher sounds like better idea.

Yes. I think this matcher will be very baseline, and can just be added where needed
(with appropriate test coverage, of course), without matcher duplication like in this current diff.

@lebedev.ri I agree, we are on the same page! Thanks!

[lebedev.ri]

Nice, i like this!
I think the test coverage is good.
But what about other equalsNode() that you didn't change?
Do some of them need this change too?

lib/Analysis/ExprMutationAnalyzer.cpp

27

(Can anyone suggest a better name maybe?
I'm not sure this is the most correct name, but i can't suggest a better alternative.)

223

Not changed?

225–227

Can these two happen with comma op?

240

Same, can this happen with comma op?

248

And here too

265

And here

279

And here

[djtodoro]

Nice, i like this!
I think the test coverage is good.

@lebedev.ri Thanks!

But what about other equalsNode() that you didn't change?
Do some of them need this change too?

For sure some of them (maybe all of them) needs usage of the new comma matcher.
I didn't check all of them, for those I checked I've created test cases.
But I agree, it should handle all possible situation.

lib/Analysis/ExprMutationAnalyzer.cpp

27

Maybe evalCommaExpr?

[lebedev.ri]

lib/Analysis/ExprMutationAnalyzer.cpp

27

Ah, yes, eval sounds better.
But, it won't always eval comma op or bailout.
It will eval if it is there.
So maybe maybeEvalCommaExpr (pun intended!).

[djtodoro]

lib/Analysis/ExprMutationAnalyzer.cpp

27

Sure, maybeEvalCommaExpr sounds better.
Or, one more suggestion is tryEvalCommaExpr ?

[lebedev.ri]

Anyways, this looks good in this state.
Thank you for working on this!

If you don't intend to immediately work on fixing the rest of , cases here,
*please* do file a bug so it won't get lost. (and link it here)

lib/Analysis/ExprMutationAnalyzer.cpp

27

try, at least to me, has the same meaning as evalCommaExpr, i.e. one might expect that if it fails, it will fail.
So not sure.

[djtodoro]

Anyways, this looks good in this state.
Thank you for working on this!

Thanks!

If you don't intend to immediately work on fixing the rest of , cases here,
*please* do file a bug so it won't get lost. (and link it here)

I already extended this in order to support the rest of direct mutations.

lib/Analysis/ExprMutationAnalyzer.cpp

27

Hmm, I agree. You are right. Let it be maybeEvalCommaExpr (for now), if there is no better suggestion.

[djtodoro]

-add support for the rest of direct mutation cases

[lebedev.ri]

Still LG.
I'm sure you have verified that there is test coverage for all the newly-supported cases.

include/clang/AST/Expr.h

public:
   bool isRelationalOp() const { return isRelationalOp(getOpcode()); }
 
   static bool isEqualityOp(Opcode Opc) { return Opc == BO_EQ || Opc == BO_NE; }
   bool isEqualityOp() const { return isEqualityOp(getOpcode()); }
 
   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:
       llvm_unreachable("Not a comparison operator.");
     case BO_LT: return BO_GE;
     case BO_GT: return BO_LE;
     case BO_LE: return BO_GT;
     case BO_GE: return BO_LT;

lib/Analysis/ExprMutationAnalyzer.cpp

 }
 
 AST_MATCHER_P(CXXForRangeStmt, hasRangeStmt,
               ast_matchers::internal::Matcher<DeclStmt>, InnerMatcher) {
   const DeclStmt *const Range = Node.getRangeStmt();
   return InnerMatcher.matches(*Range, Finder, Builder);
 }
 
+AST_MATCHER_P(Expr, skipCommaOps,
+             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;
 
 AST_MATCHER(CXXTypeidExpr, isPotentiallyEvaluated) {
   return Node.isPotentiallyEvaluated();
 }
 
 const ast_matchers::internal::VariadicDynCastAllOfMatcher<Stmt, CXXNoexceptExpr>
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(equalsNode(Exp)));
+      binaryOperator(isAssignmentOperator(),
+                     hasLHS(skipCommaOps(equalsNode(Exp))));
 
   // Operand of increment/decrement operators.
   const auto AsIncDecOperand =
       unaryOperator(anyOf(hasOperatorName("++"), hasOperatorName("--")),
-                    hasUnaryOperand(equalsNode(Exp)));
+                    hasUnaryOperand(skipCommaOps(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(skipCommaOps(equalsNode(Exp)))),
                  cxxOperatorCallExpr(callee(NonConstMethod),
                                      hasArgument(0, equalsNode(Exp))),
                  callExpr(callee(expr(anyOf(
                      unresolvedMemberExpr(hasObjectExpression(equalsNode(Exp))),
                      cxxDependentScopeMemberExpr(
                          hasObjectExpression(equalsNode(Exp)))))))));
 
   // Taking address of 'Exp'.
   // We're assuming 'Exp' is mutated as soon as its address is taken, though in
   // theory we can follow the pointer and see whether it escaped `Stm` or is
   // dereferenced and then mutated. This is left for future improvements.
   const auto AsAmpersandOperand =
       unaryOperator(hasOperatorName("&"),
                     // A NoOp implicit cast is adding const.
                     unless(hasParent(implicitCastExpr(hasCastKind(CK_NoOp)))),
-                    hasUnaryOperand(equalsNode(Exp)));
+                    hasUnaryOperand(skipCommaOps(equalsNode(Exp))));
   const auto AsPointerFromArrayDecay =
       castExpr(hasCastKind(CK_ArrayToPointerDecay),
                unless(hasParent(arraySubscriptExpr())), has(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.
   const auto AsOperatorArrowThis =
       cxxOperatorCallExpr(hasOverloadedOperatorName("->"),
                           callee(cxxMethodDecl(ofClass(isMoveOnly()),
                                                returns(nonConstPointerType()))),
                           argumentCountIs(1), hasArgument(0, 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.
   // Instantiated template functions are not handled here but in
   // findFunctionArgMutation which has additional smarts for handling forwarding
   // references.
   const auto NonConstRefParam = forEachArgumentWithParam(
-      equalsNode(Exp), parmVarDecl(hasType(nonConstReferenceType())));
+      skipCommaOps(equalsNode(Exp)),
+      parmVarDecl(hasType(nonConstReferenceType())));
   const auto NotInstantiated = unless(hasDeclaration(isInstantiated()));
   const auto AsNonConstRefArg = anyOf(
       callExpr(NonConstRefParam, NotInstantiated),
       cxxConstructExpr(NonConstRefParam, NotInstantiated),
       callExpr(callee(expr(anyOf(unresolvedLookupExpr(), unresolvedMemberExpr(),
                                  cxxDependentScopeMemberExpr(),
                                  hasType(templateTypeParmType())))),
                hasAnyArgument(equalsNode(Exp))),
       cxxUnresolvedConstructExpr(hasAnyArgument(equalsNode(Exp))));
 
   // Captured by a lambda by reference.
   // If we're initializing a capture with 'Exp' directly then we're initializing
   // a reference capture.
   // For value captures there will be an ImplicitCastExpr <LValueToRValue>.
   const auto AsLambdaRefCaptureInit = lambdaExpr(hasCaptureInit(Exp));
 
   // Returned as non-const-ref.
   // If we're returning 'Exp' directly then it's returned as non-const-ref.
   // 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 Matches =
       match(findAll(stmt(anyOf(AsAssignmentLhs, AsIncDecOperand, AsNonConstThis,
                                AsAmpersandOperand, AsPointerFromArrayDecay,
                                AsOperatorArrowThis, AsNonConstRefArg,
                                AsLambdaRefCaptureInit, AsNonConstRefReturn))
                         .bind("stmt")),
             Stm, Context);

unittests/Analysis/ExprMutationAnalyzerTest.cpp

TEST(ExprMutationAnalyzerTest, CastToConstRef) {
   EXPECT_FALSE(isMutated(Results, AST.get()));
 
   AST = buildASTFromCode("typedef const int& CIntRef;"
                          "void f() { int x; static_cast<CIntRef>(x); }");
   Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
   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, 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, LambdaDefaultCaptureByValue) {
   const auto AST = buildASTFromCode("void f() { int x; [=]() { x; }; }");
   const auto Results =
       match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
   EXPECT_FALSE(isMutated(Results, AST.get()));
 }
 
 TEST(ExprMutationAnalyzerTest, LambdaExplicitCaptureByValue) {